-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Transform annotations only if defined in current run #1241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There's no point transforming annotations that come from classfiles. It's inefficient to do so and it's also risky because it means we'd have to make sense of Scala-2 generated trees. This should avoid the error in scala#1222.
As noticed by @smarter we need to ensure that classes owning derived type params are completed, so that trees get the proper symbol attachments. This triggered when I changed annotation transformers - I have no idea whether how two could be related, though.
@smarter can you review last commit please? |
The observed error was for compile-stdlib Junit test on my machine. |
val annotTrees1 = annotTrees.mapConserve(annotationTransformer.macroTransform) | ||
val annots1 = if (annotTrees eq annotTrees1) ref.annotations else annotTrees1.map(new ConcreteAnnotation(_)) | ||
val annots1 = | ||
if (!ref.symbol.isDefinedInCurrentRun) ref.annotations // leave annotations read from class files alone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@DarkDimius It was something detected and diagnosed by @smarter. Maybe he can give an example. The test case he found was super-complicated though. The patch makes obvious sense; of course the class owner of type parameters needs to be completed before the parameters are type-checked. The only surprising thing is that this is not already always the case, but that sometimes an explicit |
@@ -46,7 +46,11 @@ object desugar { | |||
*/ | |||
override def ensureCompletions(implicit ctx: Context) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above could be updated to note that we also need to have the class itself completed.
I observed this when desugaring repeated parameters (but it can happen in a bunch of place in
So I see two ways to fix this: either make sure in LGTM. |
There's no point transforming annotations that come from
classfiles. It's inefficient to do so and it's also risky
because it means we'd have to make sense of Scala-2 generated trees.
This should avoid the error in #1222.
Review by @DarkDimius